Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BugFix: Make outside-left and outside-right block regions really responsive, resolves #266 #643

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

prasanna-lmsace
Copy link
Contributor

No description provided.

@prasanna-lmsace prasanna-lmsace marked this pull request as ready for review April 28, 2024 07:30
@abias abias force-pushed the main branch 3 times, most recently from 5696e9a to b9fa4ca Compare October 21, 2024 15:33
@abias abias force-pushed the outside-regions-fix branch from cc24267 to 23228c8 Compare November 21, 2024 10:49
@abias
Copy link
Member

abias commented Nov 21, 2024

Thank you @prasanna-lmsace for contributing this fix and thank you for your patience with our pending review.

I have just rebased and force-pushed your branch and will now review it.

Copy link
Member

@abias abias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prasanna-lmsace ,

many thanks again for working on this PR and sorry again for my long latency to review it.

I have just pushed a "Review changes" commit which is basically just housekeeping and comments / documentation.

Reviewing your changes, I did not spot major flaws on Firefox/Mac and Chrome/Mac. The outside-left and outside-right block regions are displayed on the side or wrapped properly based on the particular screen width.

As a side note: I think that the media queries / breakpoing handling for block regions have become even more complicated than they were before, but I do not have a good proposal how to simplify that, so let's keep it for now.

Having said that, I would like to highlight some breakpoint glitches where I think the proportions of the block region and the content region is not well balanced anymore.

Glitch 1a:

Browser window width: 1200px
theme_boost_union | coursecontentmaxwidth = 950px
Outside Left Region = Enabled
Outside Right Region = Enabled
theme_boost_union | blockregionoutsideleftwidth = 350px
theme_boost_union | blockregionoutsiderightwidth = 250px
theme_boost_union | outsideregionsplacement = Near Window Edges

On this window size, the outside blocks still keep their configured width.
Based on the fact that both the left and the right region are used, the content area gets really condensed:

grafik

My gut feeling is that, if (and only if) both the left and the right region is enabled and used, the blocks should already be shrunken in width to give the content area more space.

Glitch 1b:

Browser window width: 1200px
theme_boost_union | outsideregionsplacement = Next to main content area
All other settings are the same as with glitch 1a.

With this setting, the content area is even more condensed:

grafik

My change request here is the same as with glitch 1a.

Glitch 2a:

Browser window width: 990px
theme_boost_union | coursecontentmaxwidth = 950px
Outside Left Region = Enabled
Outside Right Region = Enabled
theme_boost_union | blockregionoutsideleftwidth = 350px
theme_boost_union | blockregionoutsiderightwidth = 250px
theme_boost_union | outsideregionsplacement = Next to main content area

On this window size, the outside blocks are already wrapped above and below the content area which is fine.

I have just noticed that the block regions are not fully aligned vertically with the blocks in the main area:

grafik

Would you agree that the block regions could use the same horizontal margins as the content area?

Glitch 2b:

Browser window width: 990px
theme_boost_union | outsideregionsplacement = Near Window Edges
All other settings are the same as with glitch 2a.

With this setting, it is surprising that the content area gets smaller and does not use the available width anymore:

grafik

This is different to the content area width with the "Next to main content area" setting and different to the content area width if you do not have any outside blocks enabled. I think this is just a glitch in the CSS selectors and can hopefully be solved quite quickly.


I would appreciate if you could have a look at these glitches, solve them and update the PR?
Please do not squash the PR, just add a new commit so that I just have to review your latest changes.

In addition to that, I have posted a small question to the code and would appreciate if you could answer it.

Many thanks in advance!
Alex

@@ -902,7 +941,7 @@ body.theme-boost-union-commincourse {
}

/* Additional styling for content width regions to limit the block region size on Moodle pages with limited width. */
body.limitedwidth .theme-block-region-outside-coursecontentwidth,
body.limitedwidth #theme-block-region-outside-top.theme-block-region-outside-coursecontentwidth,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain why you limited the scope of this selector to only pages where the outside-top block region exists?

@abias
Copy link
Member

abias commented Dec 6, 2024

Review

Following https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Check-list-for-peer-reviewing-patches-and-pull-requests

Documentation

  • [Y] Commit Message understandable and issue referenced (via resolves keyword)
  • [Y] Patch author correct
  • [Y] CHANGES.md appended
  • [-] README.md appended
  • [-] Credits appended
  • [Y] Appropriate Comment quantity
  • [Y] Successful Moodle PHPDoc check

version.php

  • [Y] Checked if $plugin->version increment necessary (and increment done if necessary)
  • [Y] $plugin->release untouched

lib.php

  • [-] Only necessary functions

Languages

  • [-] Only english strings
  • [-] Necessary magic strings added (e.g. capabilities)

Automated tests

  • [-] New functionality covered
  • [Y] No failing steps or scenarios

Mustache templates

  • [-] Example context exists

CSS and styles.css

  • [Y] Bootstrap styles used if possible

Duplicated code

  • [-] Duplicated code is marked properly

Github action

  • [Y] All green

Commit history and scope

  • [N] Already single commit
  • [Y] Focus on single purpose
  • [Y] No surplus files

Additional aspects for Boost Union

  • [Y] No usage of $theme->settings
  • [-] Usage of isset() checks when processing plugin settings in renderer / output code
  • [-] Usage of admin_setting_configselect instead of admin_setting_configcheckbox
  • [-] Modified mustache templates properly marked and .upstream template in place

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants